Skip to content

Unconditionally capture tokens for attributes. #77255

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Oct 24, 2020
Merged

Unconditionally capture tokens for attributes. #77255

merged 4 commits into from
Oct 24, 2020

Conversation

Aaron1011
Copy link
Member

This allows us to avoid synthesizing tokens in prepend_attr, since we
have the original tokens available.

We still need to synthesize tokens when expanding cfg_attr,
but this is an unavoidable consequence of the syntax of cfg_attr -
the user does not supply the # and [] tokens that a cfg_attr
expands to.

This is based on PR #77250 - this PR exposes a bug in the current collect_tokens implementation, which is fixed by the rewrite.

@rust-highfive
Copy link
Contributor

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 27, 2020
@Aaron1011
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Collaborator

bors commented Sep 27, 2020

⌛ Trying commit 621463923b669eb8e70c465bffa8eab63bb55b74 with merge 01460526ba12973fff687fd5d0771741d6dec0d0...

@bors
Copy link
Collaborator

bors commented Sep 27, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 01460526ba12973fff687fd5d0771741d6dec0d0 (01460526ba12973fff687fd5d0771741d6dec0d0)

@rust-timer
Copy link
Collaborator

Queued 01460526ba12973fff687fd5d0771741d6dec0d0 with parent 1ec980d, future comparison URL.

@petrochenkov
Copy link
Contributor

r? @petrochenkov

@rust-highfive rust-highfive assigned petrochenkov and unassigned eddyb Sep 27, 2020
@petrochenkov
Copy link
Contributor

Blocked on #77250.

@petrochenkov petrochenkov added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 27, 2020
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (01460526ba12973fff687fd5d0771741d6dec0d0): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

@Aaron1011
Copy link
Member Author

A lot of the slowdown is probably due to #77250

@petrochenkov
Copy link
Contributor

#77250 has landed.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Oct 21, 2020
@Aaron1011 Aaron1011 force-pushed the feature/collect-attr-tokens branch from 6214639 to 25149b8 Compare October 21, 2020 22:50
@Aaron1011
Unconditionally capture tokens for attributes.
b9b2546 
This allows us to avoid synthesizing tokens in `prepend_attr`, since we
have the original tokens available.

We still need to synthesize tokens when expanding `cfg_attr`,
but this is an unavoidable consequence of the syntax of `cfg_attr` -
the user does not supply the `#` and `[]` tokens that a `cfg_attr`
expands to.
@Aaron1011
Strip tokens before printing AST JSON
37b25e8
@Aaron1011 Aaron1011 force-pushed the feature/collect-attr-tokens branch from 25149b8 to 37b25e8 Compare October 21, 2020 23:44
@Aaron1011
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Collaborator

bors commented Oct 21, 2020

⌛ Trying commit 37b25e8 with merge c2017a52dcd6c36ff6aacd9632eed4b96f7440aa...

@bors
Copy link
Collaborator

bors commented Oct 22, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: c2017a52dcd6c36ff6aacd9632eed4b96f7440aa (c2017a52dcd6c36ff6aacd9632eed4b96f7440aa)

@rust-timer
Copy link
Collaborator

Queued c2017a52dcd6c36ff6aacd9632eed4b96f7440aa with parent 1eaadeb, future comparison URL.

@Aaron1011
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Collaborator

bors commented Oct 22, 2020

⌛ Trying commit 5c7d8d0 with merge 67a0ba996f5e1d8ba58854397300e83feaf43de2...

@Aaron1011
Copy link
Member Author

@rust-timer build 67a0ba996f5e1d8ba58854397300e83feaf43de2

@rust-timer
Copy link
Collaborator

Queued 67a0ba996f5e1d8ba58854397300e83feaf43de2 with parent a9cd294, future comparison URL.

@Aaron1011
Copy link
Member Author

cc @rust-lang/infra The try build completed, but Bors never left a comment.

@bors
Copy link
Collaborator

bors commented Oct 22, 2020

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 22, 2020
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (67a0ba996f5e1d8ba58854397300e83feaf43de2): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

@Aaron1011
Copy link
Member Author

The performance impact is now less than 0.5%

@petrochenkov: This is now ready for review

@Aaron1011 Aaron1011 mentioned this pull request Oct 23, 2020
Merged
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 24, 2020

📌 Commit 5c7d8d0 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 24, 2020
@bors
Copy link
Collaborator

bors commented Oct 24, 2020

⌛ Testing commit 5c7d8d0 with merge ffa2e7a...

@bors
Copy link
Collaborator

bors commented Oct 24, 2020

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing ffa2e7a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 24, 2020
@bors bors merged commit ffa2e7a into rust-lang:master Oct 24, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 24, 2020
Aaron1011 added a commit to Aaron1011/rust that referenced this pull request Oct 26, 2020
@Aaron1011
Remove tokens from foreign items in TokenStripper
174ed0c 
Fixes rust-lang#78398

I forgot to handle this case in rust-lang#77255
@Aaron1011 Aaron1011 mentioned this pull request Oct 26, 2020
Merged
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Oct 28, 2020
@Dylan-DPC
Rollup merge of rust-lang#78408 - Aaron1011:fix/remove-foreign-tokens…
14b4ed2 
…, r=oli-obk

Remove tokens from foreign items in `TokenStripper`

Fixes rust-lang#78398

I forgot to handle this case in rust-lang#77255
@petrochenkov petrochenkov mentioned this pull request Nov 3, 2020
Merged
@SNCPlay42 SNCPlay42 mentioned this pull request Nov 9, 2020
Closed
@jruderman jruderman mentioned this pull request Nov 13, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@petrochenkov petrochenkov

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.49.0
Development

Successfully merging this pull request may close these issues.
7 participants
@Aaron1011 @rust-highfive @rust-timer @bors @petrochenkov @eddyb @rustbot